Skip to content

feat(rich-content): unify Chat and Issue/Comment on one RichContent renderer (MUL-4922)#5578

Open
NevilleQingNY wants to merge 3 commits into
mainfrom
agent/walt/mul-4922-canonical-sanitize
Open

feat(rich-content): unify Chat and Issue/Comment on one RichContent renderer (MUL-4922)#5578
NevilleQingNY wants to merge 3 commits into
mainfrom
agent/walt/mul-4922-canonical-sanitize

Conversation

@NevilleQingNY

@NevilleQingNY NevilleQingNY commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

What

One product-level readonly renderer for Chat, Issue descriptions and Comments. A ```mermaid fence an agent emits now renders as a diagram in Chat, not a dead code block.

Two commits: ① canonical sanitize base (security convergence, no visible change) ② the RichContent convergence (this is where the user-visible fix lands).

Why

There were two product Markdown chains. Issue/Comment had Mermaid, HTML preview, lowlight code and product Mention/Attachment/LinkHover; Chat went through a generic renderer with no Mermaid/HTML dispatcher at all. Chat is where agents emit the most diagrams — the gap was worst exactly where it mattered.

Architecture

packages/views/rich-content/ is the ONE product renderer: a single ReactMarkdown pipeline, one sanitize config, one components map, one fenced-code dispatcher.

Public API is content / attachments / density / phase. Deliberately no surface prop, renderMention override, or custom code-renderer hook — each is a door a per-surface fork walks back through.

  • density is CSS only; phase is lifecycle only. Neither switches parser, plugins or semantic DOM.
  • ReadonlyContent is now a ~40-line compatibility wrapper (was 501 lines); its consumers are untouched. views/common/markdown.tsx is deleted.
  • Leaf components stay in editor/ and are imported by direct path, so Tiptap keeps reusing them and Chat doesn't pull in the Tiptap graph. Moving them is a later mechanical commit.

The three review contracts

① Live → persisted identity. The live timeline moved out of Virtuoso's Footer into a real row keyed task:<taskId>; persisted assistant rows key on the same task instead of message.id. Completion is an in-place data swap through one AssistantMessage, so an already-rendered diagram stays mounted. Test asserts same row key across handoff and that Mermaid's render count does not increase.

Consequence: the process fold previously relied on that remount to re-collapse, so the collapse is now expressed directly.

② CommonMark fence gate. Closedness comes from a real mdast parse, not a startsWith("```") scan. The gate returns offsets only and never renders — a gate that rendered would be the second renderer this change deletes. Closedness (not phase) is the gate, so a settled-but-malformed fence stays source. I verified source offsets survive rehype-raw + sanitize + katex before relying on them.

Covered edge cases the naive scan gets wrong: closer shorter than opener (```` closed by ``` is still open), tilde fences, indented fences, fences in list items, nested fences.

③ Import boundary guards. Source-level tests failing on the commit that forks: no product surface imports the generic Markdown renderer, builds its own react-markdown pipeline, configures sanitize, or branches on a fence language. Tiptap's editable NodeView is a narrow, named, documented exemption (constraint 6 — the editor owns its typing lifecycle); rewriting it is out of scope.

Five-surface Mermaid acceptance

naiyuan's fixture (corrected to a single closed fence) renders the same Mermaid leaf in all five: Chat user · Chat live assistant · Chat persisted assistant · Issue description · Comment — exercised through the real entry points (ReadonlyContent, ChatMessageList), so a Chat-only renderer regression fails the test.

Two changes worth flagging

  1. Chat code blocks move Shiki → lowlight (Howard's ratified choice), matching Tiptap/Readonly and existing hljs CSS. Expected, testable visual delta. Ligature suppression carries over via code.css instead of utility classes — mechanism changes, behavior doesn't.
  2. Chat message tests now stub react-virtuoso. Real Virtuoso renders its Footer but no data rows under jsdom's zero-height viewport (I probed this directly); since the live timeline is now a row, the MUL-3960 regression tests couldn't see it. Test-environment artifact, not a production regression.

Verification

  • pnpm typecheck — 6 workspaces, pass
  • pnpm lint — 0 errors, none on touched files
  • packages/views vitest — 240 files, 2743 tests, all passing

Not verified: no browser/visual check of the lowlight restyle or scroll-position behaviour under real virtualization — those need a running app.

Closes MUL-4922

🤖 Generated with Claude Code

@vercel

vercel Bot commented Jul 17, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
multica-docs Ready Ready Preview, Comment Jul 20, 2026 6:03am

Request Review

NevilleQingNY and others added 2 commits July 20, 2026 13:32
…rs (MUL-4922)

Phase 1 of the RichContent convergence: collapse the duplicated security
base shared by the two product-level Markdown chains.

Chat (packages/ui/markdown/Markdown.tsx) and Issue/Comment
(packages/views/editor/readonly-content.tsx) each carried a verbatim fork
of the rehype-sanitize schema and urlTransform, and the forks had already
drifted: readonly whitelisted <mark> for `==highlight==`, chat did not.
A security-relevant allow-list maintained in two places means every future
XSS fix has to land twice, and missing one is a hole — this is the hardest
reason for the sweep, ahead of the user-visible feature drift.

- Extract markdownSanitizeSchema + markdownUrlTransform into
  packages/ui/markdown/sanitize.ts and export from the package index.
  Both chains now import the single copy; no local forks remain.
- The canonical schema is the union, so chat gains the <mark> tag name.
  This is the one intentional behavior delta: <mark> is inert and admits
  no attributes, and chat needs it anyway once ==highlight== converges.
- Annotate the schema as rehype-sanitize's Options: exporting it makes the
  previously-inferred hast-util-sanitize type unnameable across packages.

Adds a cross-surface contract test that runs one set of security fixtures
(script, event handlers, javascript: href, data:image vs data:text/html,
mark, slash://) through BOTH surfaces and asserts identical outcomes —
the mechanism that stops a third fork from growing back.

Code-block rendering is deliberately not asserted cross-surface yet: chat
highlights with Shiki, readonly with lowlight, so emitted class tokens
still differ. Converging them is the RichCodeBlock phase and needs the
highlight-engine decision first; only the schema-level allow-list is
shared here.

Verified: pnpm typecheck (6 workspaces), pnpm lint (0 errors),
pnpm vitest run in packages/views (228 files, 2665 tests, all passing).

Co-authored-by: multica-agent <github@multica.ai>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nt (MUL-4922)

Phase 2+3 of the convergence. Chat now renders agent output through the same
product renderer as Issue descriptions and Comments, so a ```mermaid fence an
agent emits is a diagram in Chat — not a dead code block.

Before this there were two product Markdown chains. Issue/Comment had Mermaid,
HTML preview, lowlight code, product Mention/Attachment/LinkHover; Chat went
through a generic renderer with no Mermaid/HTML dispatcher at all. Chat is
where agents emit the most diagrams, so the gap was most visible exactly where
it mattered.

Architecture
- packages/views/rich-content/ holds the ONE product renderer: a single
  ReactMarkdown pipeline, one sanitize config, one components map, one fenced
  -code dispatcher. Public API is content/attachments/density/phase — no
  `surface` prop, no renderMention override, no custom code-renderer hook,
  because each is a door a per-surface fork walks back through.
- `density` is CSS only; `phase` is lifecycle only. Neither switches parser,
  plugins or the semantic DOM, so all surfaces emit the same blocks.
- ReadonlyContent is now a ~40-line compatibility wrapper (was 501 lines);
  its consumers are untouched. views/common/markdown.tsx is deleted.
- Leaf components (Mermaid, HTML preview, lowlight code) stay in editor/ and
  are imported by direct path, so Tiptap keeps reusing them and Chat does not
  pull in the editor's Tiptap graph. Moving them is a later mechanical commit.

Streaming fence gate
Rich blocks upgrade only when the fence is CLOSED, judged by a real CommonMark
parse (mdast) rather than a `startsWith("```")` scan. A half-streamed fence
renders as source, so Mermaid never parses a partial diagram and no iframe is
created for HTML still arriving. Closedness — not `phase` — is the gate, so a
settled-but-malformed fence stays source too. The gate returns offsets only; it
never renders, because a gate that rendered would be the second renderer this
change exists to delete. Verified that source offsets survive rehype-raw +
sanitize + katex before relying on them.

Live -> persisted row identity
The live timeline moved out of Virtuoso's Footer into a real row keyed
`task:<taskId>`, and persisted assistant rows key on the same task instead of
`message.id`. Completion is now an in-place data swap through one
AssistantMessage component, so an already-rendered diagram or iframe stays
mounted and is not re-run. The process fold previously relied on that remount
to re-collapse, so the collapse is now expressed directly.

Notes
- Chat code blocks move from Shiki to lowlight (Howard's ratified choice),
  matching Tiptap/Readonly and the existing hljs CSS. Ligature suppression
  carries over via code.css instead of utility classes.
- Chat message tests now stub react-virtuoso: real Virtuoso renders its Footer
  but no data rows under jsdom's zero-height viewport, and the live timeline is
  a row now.

Tests: five-surface Mermaid parity (Chat user / live / persisted, Issue
description, Comment), streaming open->closed->settled with mount counting,
live->persisted no-remount, htmlbars/mermaidx not misdispatched, sandbox and
data-URI security regressions, CommonMark fence edge cases (shorter closer,
tilde, indented, nested, in-list), and source-level import boundary guards.

Verified: pnpm typecheck (6 workspaces), pnpm lint (0 errors),
packages/views vitest 240 files / 2743 tests all passing.

Co-authored-by: multica-agent <github@multica.ai>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@NevilleQingNY
NevilleQingNY force-pushed the agent/walt/mul-4922-canonical-sanitize branch from b3f0a26 to 0126b7b Compare July 20, 2026 05:34
@NevilleQingNY NevilleQingNY changed the title refactor(markdown): single canonical sanitize schema for both renderers (MUL-4922) feat(rich-content): unify Chat and Issue/Comment on one RichContent renderer (MUL-4922) Jul 20, 2026
… blocks (MUL-4922)

Two review follow-ups.

1. Dangling package export
`packages/views` still exported "./common/markdown" after the file was
deleted in the RichContent convergence. TypeScript resolves against the
source tree so typecheck and unit tests both passed; the subpath would only
fail when a consuming app bundled it. No consumer imports it, so this was
latent rather than broken in practice.

Removes the entry and adds packages/views/rich-content/package-exports.test.ts,
which walks every workspace package.json and asserts each export target
exists (wildcards checked by directory prefix). Scoped repo-wide because the
failure mode belongs to package.json, not to this package. Verified the guard
actually fails by re-adding the dangling entry before committing.

2. Near-viewport lazy shell for Mermaid / HTML
Implements the deferred performance contract rather than requesting an
exemption. LazyRichBlock defers each rich leaf until it is within 800px of
the viewport, then latches: once mounted it is never unmounted, so scrolling
past does not re-run Mermaid, rebuild the sandboxed iframe, or discard the
viewer's pan/zoom state.

The stable-size requirement is handled by reserving the block's expected
height before AND after mount, so a block never measures 0px off-screen and
then jumps — the churn that makes a virtualized list mis-estimate item sizes.
The reservation is not a local guess: reservedMermaidHeightPx() reuses the
existing session layout cache (real height on a cache hit, else the documented
280px skeleton) and HTML_BLOCK_PREVIEW_HEIGHT_PX is the pixel twin of the
preview iframe's fixed h-[480px]. Both are exported from the leaves so there
is one source of truth per block type.

Environments without IntersectionObserver (jsdom, SSR) mount eagerly, which is
today's behaviour; rendering nothing would not be.

Verified in real Chromium (jsdom has no IntersectionObserver, so unit tests
only exercise the eager fallback):
- Non-virtualized Issue/Comment with 25 diagrams: 3 mounted, 22 deferred on
  load; after scrolling, mounted grows 3 -> 6 and never drops, confirming the
  latch. This is where the win is real.
- Chat gains less: Virtuoso already caps the DOM to ~4 rows, so only 1 of 4
  shells was deferred. Stated rather than overclaimed.
- Live -> persisted handoff re-checked with the shell in place: scrollTop
  220 -> 220 (delta 0), the tagged diagram DOM node survived, and the run
  reached the persisted state. Contract 1 is not regressed.

Tests: 6 lazy-shell cases (defer, mount-on-approach, latch/no-unmount, equal
reserved height before and after mount, root margin exceeding the chat list's
own overscan, no-IntersectionObserver fallback) plus 3 export-guard cases.

Verified: pnpm typecheck (6 workspaces), pnpm lint (0 errors),
packages/views vitest 242 files / 2752 tests all passing.

Co-authored-by: multica-agent <github@multica.ai>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant